Improve test suite, reorganize to platform classes #26
Conversation
Signed-off-by: Bernát Gábor <gaborjbernat@gmail.com>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Codecov Report
@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 83.69% 89.64% +5.94%
==========================================
Files 7 12 +5
Lines 411 425 +14
Branches 85 43 -42
==========================================
+ Hits 344 381 +37
+ Misses 47 31 -16
+ Partials 20 13 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
gaborbernat
requested review from
Julian,
ofek and
RonnyPfannschmidt
as code owners
| path = os.environ["XDG_DATA_DIRS"] | ||
| else: | ||
| path = f"/usr/local/share{os.pathsep}/usr/share" | ||
| return self._with_multi_path(path) |
There was a problem hiding this comment.
why not work with split paths here and only join the result
There was a problem hiding this comment.
Can you detail? I've just kept the existing method, but we can switch if you propose something better.
src/platformdirs/windows.py
Outdated
|
|
||
| def _path_with_app_version(self, path: str, *, opinion_value: Optional[str] = None) -> str: | ||
| if self.appname: | ||
| assert self.appname is not None |
There was a problem hiding this comment.
this seems redundant at first glance
There was a problem hiding this comment.
Hmmm, kept existing logic, but yeah this can be removed.
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
|
This looks fine but I'd much prefer to keep the conditional detection logic. I have a bias b/c I write a lot of CLIs where load times are important, but really why should we import every module at the root when only one will be used? |
Can you provide some metrics of a performance difference? I don't think there's a significant difference in importing three small files. What you're talking about is premature optimization? |
|
Not an issue currently, but we'd have to careful with imports in each module. Say for example we're on not-Android and a new Python release made importing
I don't think so as the 2 approaches are so different that going back to the current one by removing |
I'd definitely keep is_active. At that point, you'd have different solutions to use import functools locally where it's needed. |
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
|
Unfortunately im quite stretched today and can't give this quite the time it deserves |
| class MacOS(PlatformDirsABC): | ||
| """ | ||
| Platform directories for the macOS operating system. Follows the guidance from `Apple documentation | ||
| <https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/MacOSXDirectories/MacOSXDirectories.html>`_. |
There was a problem hiding this comment.
Can you move the URL to the end of the parragraph as done here?
This helps keep the docstring more readable in it's raw format.
There was a problem hiding this comment.
I disagree in this instance, I prefer to keep links inline 👍
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Managed to close #25 by mistake so opening again.